fixing the one test that failed turned out to more complicated than I imagined.

I made some non-trivial changes which I will try to explain inline.

please code review @mkcor @chriddyp @cpsievert

http://ropensci.github.io/plotly-test-table/tables/8e68d8abfe1137ac265e8c80f92502b3fa3c7892/big.html shows some untested regressions. I will add tests and make the fixes.

still need to fix these test failures

1. Failure (@test-cookbook-lines.R#215): Add colored lines for the mean xval of each group 
tr$showlegend isn't false

2. Failure (@test-cookbook-lines.R#215): Add colored lines for the mean xval of each group 
tr$showlegend isn't false

3. Failure (@test-cookbook-lines.R#270): geom_line -> 2 more traces ------------
tr$showlegend is not identical to expected. Differences: 
1 element mismatch

4. Failure (@test-cookbook-lines.R#270): geom_line -> 2 more traces ------------
tr$showlegend is not identical to expected. Differences: 
1 element mismatch

Also I'm wondering if anybody has ideas about how to best translate this to plotly?

http://ropensci.github.io/plotly-test-table/tables/8e68d8abfe1137ac265e8c80f92502b3fa3c7892/cookbook-lines-bar-dodge-color-error.html

also this one?

http://ropensci.github.io/plotly-test-table/tables/8e68d8abfe1137ac265e8c80f92502b3fa3c7892/cookbook-lines-bar-dodge-color-err4.html

all the tests I wrote pass now, but there are still the visual problems I mentioned above which I am not sure how to convert to plotlys. Any ideas?

It seems like those bar lines are a ggplot2 error bars hack.

# Draw separate hlines for each bar. First add another column to dat
dat$hline <- c(9,12)
dat
#>        cond result hline
#> 1   control   10.0     9
#> 2 treatment   11.5    12

# Need to re-specify bp, because the data has changed
bp <- ggplot(dat, aes(x=cond, y=result)) +
    geom_bar(position=position_dodge(), stat="identity")

# Draw with separate lines for each bar
bp + geom_errorbar(aes(y=hline, ymax=hline, ymin=hline), colour="#AA0000")

# Make the lines narrower
bp + geom_errorbar(width=0.5, aes(y=hline, ymax=hline, ymin=hline), colour="#AA0000")

At some point, ggplotly was merging geom_errors with geom_bar and geom_line traces if they shared the same x and y data. In this case, these geoms don't share x and y data, so either we keep them attached to invisible bar traces or we convert them to plotly's new layout.shape.line objects.

Option 1 - Attaching them to invisible bar traces
Pros

• position is really straightforward
• ggplot2 has width relative to bar width (whereas we just have the pixel width). If we had relative widths, specifying width would be really straightforward too

Cons

• Since we do width in pixels right now, it'll be pretty annoying to compute the width exactly right. We might be able to pull it out of the plotly.js computations though

Option 2 - Making them separate line objects
Pros

• Semantically it seems more like what the user wanted
• Computing width is possible with data coordinates and bargap and bargroupgap computations

Cons

• Computing position will be a little annoying with bargap and bargroupgap
• There won't be a legend entry for these lines
• The logic is a little weird:
  • if the geom_errorbar's ymin=ymax=y then we convert to layout.line.width
  • else if the geom_errorbar shares y with another bar or line we merge the traces
  • else we keep it attached to a hidden scatter or bar trace

Here's an example with lines, and bargap and bargroupgap set to 0

{
    'data': [
        {
            'x': ['Cats', 'Dogs'],
            'y': [4, 6],
            'type': 'bar',
            'name': 'Series 1'
        },
        {
            'x': ['Cats', 'Dogs'],
            'y': [5, 7],
            'type': 'bar',
            'name': 'Series 2'
        }
    ],
    'layout': {
        'bargap': 0,
        'shapes': [
            {
                'type': 'line',
                'x0': -0.5,
                'x1': 0.5,
                'y0': 4.5,
                'y1': 4.5,
                'line': {
                    'color': 'blue',
                    'width': 4,
                    'dash': 'dash'
                }
            },                
            {
                'type': 'line',
                'x0': 0.5,
                'x1': 1.5,
                'y0': 5.5,
                'y1': 5.5,
                'line': {
                    'color': 'red',
                    'width': 4,
                    'dash': 'dot'
                }
            },
            {
                'type': 'line',
                'x0': 0.5,
                'x1': 1,
                'y0': 6.5,
                'y1': 6.5,
                'line': {
                    'color': 'green',
                    'width': 4,
                    'dash': 'dashdot'
                }
            },
            {
                'type': 'line',
                'x0': 0,
                'x1': 0.5,
                'y0': 7.0,
                'y1': 7.0,
                'line': {
                    'color': 'orange',
                    'width': 4,
                    'dash': 'solid'
                }                    
            }                
        ]
    }
}

So, it seems like keeping them as invisible error bars and increasing the error bar width might be the easiest thing to do right now. If/when we error bar widths relative to error bar widths, this'll be really straightforward.
Of course, I'm not sure if it is possible to extract anything else out of ggplot2 that would make this easier like the data coordinates or widths of the lines

Also looks like there is a regression with legend hide/show

Hi @tdhock sorry for not being on R lately... Thanks for this work. I believe all the TODO comments should belong to a separate PR: I mean, it's a discussing worth having, but it shouldn't pollute something else you're fixing now.
Also, can you please follow the style guide? Again, comments start with one #.

@cpsievert I think the errorbars are green in the plotly plot because they correspond to the third trace, and green is the third default colour in plotly... So maybe we want to be more manual about it and convert the colour (black being the fallback).

Funnily enough, we haven't bothered (yet) with the first default colour, which is black in ggplot and blue in plotly (e.g., http://ropensci.github.io/plotly-test-table/tables/96f0d4073aabf455731b0db3775c41d9830a24e6/cookbook-lines-basic-bar.html).

@mkcor good to know, I've noticed the default color/fill are different on several tests. If you them to match, just say the word and I'll take care of it :)

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/60974677

On TravisCI, commit 9ab52e1 was successfully merged with d9ecaf4 (master) to create 9abb9de. A visual testing table comparing d9ecaf4 with 9abb9de can be found here:
http://ropensci.github.io/plotly-test-table/tables/9abb9dea695bd9d8320629392c2b2f5be87e1b49/index.html

@chriddyp @mkcor this looks ready to merge now. I can work on a more general fix on default colors/sizing in another pull request.

@cpsievert Okay, looking now. I'm confused though, because files DESCRIPTION and NEWS shouldn't be edited before getting the +1 (since you can't tell which day you'll be getting that +1).

Yea, they were edited before I merged with master, so I had to resolve those conflicts...

@cpsievert [default color/fill] No, thanks. Blue looks better, plus we've never heard anybody complain about it. ;)

@cpsievert Okay... But "5 May 2015" lies in the future so... How can it be something we would want or something we would get from somewhere?

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/61334002

On TravisCI, commit d142570 was successfully merged with f4bc6b9 (master) to create 1cf9b72. A visual testing table comparing f4bc6b9 with 1cf9b72 can be found here:
http://ropensci.github.io/plotly-test-table/tables/1cf9b723480c9ab454c12e51d5c5c4fd6be61bad/index.html

I'll pull this branch and do the stylistic fixes.

@cpsievert Let either of us +1 and the other will merge...

FYI, I think most of those style changes are due to the "Strip trailing horizontal space" option (which I'll turn on for this project)

You mean, turn off (unchecking the box)? You're right, I always left this box unchecked so I assumed it was RStudio's default. We've had this conversation in the past: #151 (comment) and subsequent comments.

Oh, yes, sorry, I must've not edited/saved those files, so they were still left over from Toby's commits.

The message below was automatically generated after build https://travis-ci.org/ropensci/plotly/builds/61377823

On TravisCI, commit 4cfedf9 was successfully merged with ae947fc (master) to create 0832b38. A visual testing table comparing ae947fc with 0832b38 can be found here:
http://ropensci.github.io/plotly-test-table/tables/0832b38fdce185d6fdfb28750cfcc2de2ceab571/index.html

Looks good! +1

I had to restart one of the builds due to a silly Travis error, but this is good to go, please merge @mkcor 👍